fix: forward GH_AW_INPUT_* to MCP container env for dynamic safe-outputs config#48099
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…uts config Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes dynamic safe-output inputs by forwarding GH_AW_INPUT_* variables toward the MCP runtime and improving unresolved-placeholder diagnostics.
Changes:
- Extracts and forwards input-derived safe-output environment variables.
- Adds runtime diagnostics and regression coverage.
- Updates release metadata and workflow skill references.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/mcp_setup_generator.go |
Extracts safe-output input variables. |
pkg/workflow/mcp_setup_gateway.go |
Forwards variables to the outer gateway container. |
actions/setup/js/safe_outputs_config.cjs |
Logs unresolved input placeholders. |
pkg/workflow/safe_outputs_dynamic_allowed_repos_test.go |
Adds compilation regression assertions. |
.github/skills/agentic-workflows/SKILL.md |
Adds the release-workflow reference. |
.changeset/fix-safe-outputs-dynamic-input-mcp-container.md |
Documents the patch. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Medium
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (3 tests)
Verdict
|
There was a problem hiding this comment.
The fix is correct and well-implemented. The root cause (GH_AW_INPUT_* vars missing from the docker -e allowlist) is clearly identified and addressed at both the compiler level (Go) and with appropriate diagnostic logging (JS). Tests are updated and a focused regression test added.
Two pre-existing review comments cover the remaining gaps:
- Nested-container forwarding (inner containers do not inherit
-evars from the outer MCP gateway) - Missing JS unit-test coverage for the new unresolved-placeholder warning path
No additional blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 21.4 AIC · ⌖ 4.51 AIC · ⊞ 5K
Design Decision Gate - ADR RequiredThis PR makes significant changes to core business logic (144 new lines in Draft ADR committed: This PR cannot merge until an ADR is linked in the PR body. What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
REQUEST_CHANGES — one medium correctness issue must be fixed before merging.
Blocking issue
The new collectUnresolvedInputPlaceholders diagnostic in safe_outputs_config.cjs only logs and continues. Since resolveEnvPlaceholders uses ?? match as its fallback, an unresolved ${GH_AW_INPUT_BASE_BRANCH} is preserved verbatim in the parsed config. The safe-outputs MCP server then receives the literal placeholder as the base_branch value — the same broken behavior the PR is fixing — just with a warning attached. The check needs to throw after logging so the failure is explicit rather than silent.
Other observations (non-blocking)
- The Go changes are correct:
-e VARNAME(without=value) is the standard Docker pattern for inheriting a value from the host process environment, and the stepenv:block supplies that value on the runner. The approach is sound. extractSafeOutputsInputEnvVarscorrectly filters toGH_AW_INPUT_*keys; the nil-on-empty return is consistent with the rest of the codebase.- The regression test
TestSafeOutputsDynamicBaseBranchPassedToMCPContaineris well-structured and covers the exact failure scenario.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 · 52.1 AIC · ⌖ 4.74 AIC · ⊞ 5.7K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — the fix is well-targeted and accompanied by a solid regression test. One issue worth addressing before merge.
📋 Key Themes & Highlights
Key Themes
- Diagnostic severity: the new
collectUnresolvedInputPlaceholderswarning is emitted atserver.debuglevel, which is suppressed in normal runs — it won't help anyone diagnose a misconfigured deployment where this code path is actually reached - Test coverage gap (noted in existing review):
safe_outputs_config.test.cjshas no coverage for the newcollectUnresolvedInputPlaceholderspath - Container scope concern (noted in existing review): the MCP gateway container forwarding only helps the outer gateway; worth confirming the safe-outputs server actually reads env vars from the container context it runs in
Positive Highlights
- ✅ Clean extraction of
extractSafeOutputsInputEnvVars— single responsibility, easy to test - ✅ Regression test
TestSafeOutputsDynamicBaseBranchPassedToMCPContainercovers the exact failure scenario end-to-end - ✅ Updated test assertions now verify both step env AND docker
-eflags — the right invariants to protect - ✅ Good inline comments explaining why each piece is necessary
- ✅ Changeset entry accurately describes the impact
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 30.8 AIC · ⌖ 4.7 AIC · ⊞ 6.7K
Comment /matt to run again
|
@copilot please run the
|
No new actionable items. Current state (branch HEAD
Nothing further is actionable from the agent side. A maintainer must dismiss the stale |
|
@copilot please run the
|
Branch is already up to date with
|
|
@copilot please run the
|
PR Finisher pass complete. Current state (branch HEAD
Actions taken this pass: no code changes needed — validated local checks only. Hand-off: A maintainer must either comment |
|
@copilot please run the Recent context:
|
This PR only needs maintainer cleanup of the stale blocking review. Here's the exact state: All inline threads resolved (confirmed via API):
The only blocker is the stale Smallest maintainer action set:
No fresh code issues remain on the current HEAD |
|
@copilot please run the Unresolved feedback to close out (newest first):
If all review threads are actually resolved already, say so clearly so maintainers can dismiss stale blocking reviews.
|
All four threads are already
This PR only needs maintainer cleanup of the stale Smallest remaining action set for maintainers:
|
🤖 PR Triage
Score breakdown: Impact 42/50 · Urgency 24/30 · Quality 16/20 Rationale:
|
|
🎉 This pull request is included in a new release. Release: |
Since v0.80.0, the safe-outputs MCP server runs in a Docker container with a filtered
-eallowlist.GH_AW_INPUT_*vars were never added to that allowlist, so${GH_AW_INPUT_BASE_BRANCH}-style placeholders inconfig.jsonremain unresolved inside the container — causingcreate_pull_requestto fail withNo remote refs available for merge-base calculationwhen using any dynamic safe-outputs field likebase-branch: ${{ inputs.base_branch }}.Changes
pkg/workflow/mcp_setup_generator.goextractSafeOutputsInputEnvVars(safeOutputConfig)— extracts allGH_AW_INPUT_*name→expression pairs referenced by the safe-outputs config and passes them togenerateMCPGatewaySetup.pkg/workflow/mcp_setup_gateway.gowriteMCPGatewayStepEnvnow also emitsGH_AW_INPUT_*: ${{ inputs.* }}in the Start MCP Gateway stepenv:block, so the runner process holds the values whendocker runis invoked.appendMCPGatewaySafeOutputsInputEnvFlagsappends-e GH_AW_INPUT_*to the docker run command so the container inherits those values.The compiled output now looks like:
actions/setup/js/safe_outputs_config.cjscollectUnresolvedInputPlaceholders()detects and logs any${GH_AW_INPUT_*}that remains unresolved at load-time, so failures surface with a clear message instead of a cryptic merge-base error.pkg/workflow/safe_outputs_dynamic_allowed_repos_test.goGH_AW_INPUT_*appears in both the Generate Safe Outputs Config and Start MCP Gateway step env blocks, and that-e GH_AW_INPUT_*is present in the docker run command.TestSafeOutputsDynamicBaseBranchPassedToMCPContainerregression test for the exact issue scenario (base-branch: ${{ inputs.base_branch }}).run: https://github.com/github/gh-aw/actions/runs/30207935610
Run: https://github.com/github/gh-aw/actions/runs/30221620246
Run: https://github.com/github/gh-aw/actions/runs/30223671533
Run: https://github.com/github/gh-aw/actions/runs/30224763533
Run: https://github.com/github/gh-aw/actions/runs/30228781602
Run: https://github.com/github/gh-aw/actions/runs/30230557448
Run: https://github.com/github/gh-aw/actions/runs/30233154470
Run: https://github.com/github/gh-aw/actions/runs/30236419434
Run: https://github.com/github/gh-aw/actions/runs/30240308794